Skip to content

Fix _load_pretrained_model#24200

Merged
SunMarc merged 1 commit intohuggingface:mainfrom
SunMarc:fix_test_marian
Jun 12, 2023
Merged

Fix _load_pretrained_model#24200
SunMarc merged 1 commit intohuggingface:mainfrom
SunMarc:fix_test_marian

Conversation

@SunMarc
Copy link
Member

@SunMarc SunMarc commented Jun 12, 2023

What does this PR do ?

Fixes the following test from my old PR Add check for tied parameters (#24029):
RUN_SLOW=1 python3 -m pytest -v tests/models/marian/test_modeling_marian.py::TestMarian_FI_EN_V2::test_batch_generation_en_fr

@SunMarc SunMarc requested review from sgugger and ydshieh June 12, 2023 14:38
sgugger
sgugger reviewed Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain a bit more why we need to remove this line? Thanks!

@SunMarc
Copy link
Member Author

SunMarc commented Jun 12, 2023

This line what something I added in my previous PR thinking that we forgot to tie the weights. But I guess it was done intentionally as I see that one test failed and generated garbage value. Here the error that we get from the test:

tests/models/marian/test_modeling_marian.py:433: in _assert_generated_batch_equal_expected
    self.assertListEqual(self.expected_text, generated_words)
E   AssertionError: Lists differ: ['I like to read books', 'I like watching football'] != ['obliterat obliterat obliterat obliterat o[1345 chars]ɰɰɰ']
E   
E   First differing element 0:
E   'I like to read books'
E   'obliterat obliterat obliterat obliterat o[1214 chars]erat'
E   
E   Diff is 1556 characters long. Set self.maxDiff to None to see it.

Could you explain a bit more why we need to remove this line? Thanks!

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 12, 2023 •
edited
Loading

The documentation is not available anymore as the PR was closed or merged.

ydshieh
ydshieh approved these changes Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@ydshieh ydshieh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole marian test suite now pass. Since you mentioned that line is added in the previous PR, the change here LGTM. Leave @sgugger to have a final judge though.

Thank you for the quick fix.

sgugger
sgugger approved these changes Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very weird that this changes anything one way or the other but let.s go.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 12, 2023 •
edited
Loading

BTW, let's make the PR title a bit more precise, sth like fix _load_pretrained_model or anything you think more precise, @SunMarc .
Thanks!

@ydshieh ydshieh changed the title Fix test Fix _load_pretrained_model Jun 12, 2023
@SunMarc
Copy link
Member Author

SunMarc commented Jun 12, 2023

Yeah, I will explore a little bit more why we have this weird behavior before merging.

@SunMarc SunMarc merged commit 08ae37c into huggingface:main Jun 12, 2023
@SunMarc SunMarc deleted the fix_test_marian branch June 12, 2023 15:31
@ydshieh ydshieh mentioned this pull request Jun 20, 2023
Merged
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
@SunMarc @novice03
Fix _load_pretrained_model (huggingface#24200)
6642ddd 
Fix test
@patrickvonplaten patrickvonplaten mentioned this pull request Aug 2, 2023
Merged
5 tasks
@zucchini-nlp zucchini-nlp mentioned this pull request Apr 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ydshieh ydshieh ydshieh approved these changes

+1 more reviewer

@sgugger sgugger sgugger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SunMarc @HuggingFaceDocBuilderDev @ydshieh @sgugger